fix: preserve path separators in create command argument#526
Conversation
The display_information.name and bot_user.display_name fields in manifest files were set to the kebab-case directory name (e.g. "my-app") instead of a human-readable title case name (e.g. "My App").
…ebab Addresses review feedback — instead of converting the kebab-case directory name to title case (which mangles names like "TIM" or "Translator - Translate Languages"), pass the original user-provided app name through to manifest display fields. This preserves the user's exact casing and formatting.
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
The getAppDirName function was normalizing the entire input including path separators, so `slack create path/to/my-app` produced a flat directory `path-to-my-app` instead of preserving the path structure. Adds parseAppPath which splits user input into path prefix and basename, only kebab-casing the basename. The display name for manifests is derived from the raw basename (preserving original casing). Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #526 +/- ##
==========================================
+ Coverage 71.30% 71.32% +0.01%
==========================================
Files 222 222
Lines 18695 18714 +19
==========================================
+ Hits 13331 13348 +17
- Misses 4184 4186 +2
Partials 1180 1180 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Verify that the parent directory path exists (we will not create missing directories) | ||
| if exists, err := parentDirExists(dirPath); err != nil { | ||
| return "", err | ||
| } else if !exists { | ||
| return "", slackerror.New(slackerror.ErrAppDirectoryAccess) | ||
| } |
There was a problem hiding this comment.
🔬 thought: So curious! I thought creating missing directories was supported but I am wrong!
There was a problem hiding this comment.
yes very curious! i thought so too but i think it would be nice to have 🤔 perhaps in followups 🚀
| // Get the app selection and accompanying app directory name (this may change when we find the unique directory name) | ||
| appDirName, err := getAppDirName(createArgs.AppName) | ||
| // Parse the app name input into a directory path and display name | ||
| appPath, displayName, err := parseAppPath(createArgs.AppName) |
There was a problem hiding this comment.
🐛 issue: I'm finding the app path can be changed with the "--name" flag which is unexpected as:
$ slack create path/to/my-app --name "APPPP"
📂 Created a new Slack project
Cloning template slack-samples/bolt-js-starter-template
To path ~/programming/tools/slack-cli/apppp
🔍 thought: I'd instead expect the argument path to be preferred here with the name being applied to the app itself? I'm unsure if separating these functions makes this more clear but think this case is good to test too!
There was a problem hiding this comment.
hmmm i thought the --name flag was always preferred? this was introduced with the name flag in #327
but thanks for catching this because i would also expect --name to override the display name and leave the path alone
There was a problem hiding this comment.
@srtaalej Ahh we might be finding nuance of this command 😓 I recall these discussion too and didn't account for these edges...
The --name flag now only sets the manifest display name, preserving any path provided via the positional argument for directory placement.
zimeg
left a comment
There was a problem hiding this comment.
🔮 @srtaalej This gets to a great place! I'm leaving a handful more comments that I hope untangle some arguments but find this is a nice fix-
- Uppercase directories: Earlier change lowercased path to directories too in normalization but downstream issues are noticed from this, albeit with workarounds. I note that it might be a nice change for a separate PR!
- App path placement: The "app name" argument feels outdated now that we're supporting "display name" too. A more meaningful change to perhaps make in this PR centralizes some of this logic but I think we can save other refactors?
I'm approving now but feel free to rerequest review if refactors feel awkward!
| return "", fmt.Errorf("app name is required") | ||
| } | ||
|
|
||
| // Normalize to kebab-case: lowercase, replace non-alphanumeric with dashes, collapse, and trim |
There was a problem hiding this comment.
| // Normalize to a variation of kebab-case: replace non-alphanumeric with dashes, collapse, and trim |
🌠 issue(non-blocking): The lowercasing might be confusing to CI that creates apps with a random name. We might consider keeping uppercase to match inputs that make a valid directory?
$ pwd
~/Documents
$ slack create MyApp
...
To path ~/Documents/myapp
|
|
||
| // Normalize to kebab-case: lowercase, replace non-alphanumeric with dashes, collapse, and trim | ||
| appName = strings.TrimSpace(appName) | ||
| appName = strings.ToLower(appName) |
There was a problem hiding this comment.
🪓 suggestion(non-blocking): With the above comment I share this.
| // --name flag overrides the manifest display name but preserves any path | ||
| // from the positional argument. When no positional arg is given (e.g. | ||
| // "slack create --name APPPP"), the name flag also becomes the directory | ||
| // path since there's nothing else to derive it from. | ||
| displayNameOverride := "" | ||
| if nameFlagProvided { | ||
| appNameArg = createAppNameFlag | ||
| displayNameOverride = createAppNameFlag | ||
| if appNameArg == "" { | ||
| appNameArg = createAppNameFlag | ||
| } |
There was a problem hiding this comment.
🪴 suggestion: We might want to move this alongside logic below with some shuffling:
slack-cli/cmd/project/create.go
Lines 148 to 166 in 7e4d486
| Template: template, | ||
| GitBranch: createGitBranchFlag, | ||
| Subdir: subdir, | ||
| AppName: appNameArg, |
There was a problem hiding this comment.
🪬 thought: The meaning of these arguments has inverted for me and I now think about this variable as the app path that might be used for the app name defaults. The path separators of this PR must have inspired such idea!
🔮 suggestion(follow-up): We might find it useful to update this but I'd like to know if I'm thinking about this right?
| AppName: appNameArg, | |
| AppPath: appPathArg, |
| // from the positional argument. When no positional arg is given (e.g. | ||
| // "slack create --name APPPP"), the name flag also becomes the directory | ||
| // path since there's nothing else to derive it from. | ||
| displayNameOverride := "" |
There was a problem hiding this comment.
| displayNameOverride := "" | |
| displayName := "" |
🔬 suggestion: I understand within pkg this overrides a parsed path but that term might be confusing from this calling code.
Changelog
semver:patch— Thecreatecommand preserves path separators when a nested path is provided as the project directory. The--nameflag now only overrides the app's display name in the manifest without replacing the path argument.Summary
slack create path/to/my-appregression where path separators were being replaced with dashes, creating a flatpath-to-my-appdirectory instead of preserving thepath/to/my-appstructureparseAppPathhelper that splits user input into path prefix + basename, only kebab-casing the basename--nameflag now sets only the manifest display name, preserving any path from the positional argumentBehavior
Stacks on #521 which handles the display name vs dir name separation.
Test plan
TestParseAppPath— 12 cases covering simple names, paths, absolute paths, dot-prefixed, trailing slashes, and error casesTestGetProjectDirectoryNamestill passes (single-component names)go test ./...) passes--nameflag tests to verify path is preserved and only display name is overriddenslack create path/to/my-appwith existingpath/to/directory → verify nested structure created and manifest hasmy-appslack create "My App"→ verify dir ismy-app, manifest display isMy Appslack create path/to/my-app --name "APPPP"→ verify dir ispath/to/my-app, manifest display isAPPPP